Conversation
… with mediawiki version>1.35
…manticProperties, props are saved as specialProps
…ditional datatypes (edtf, localMedia)
removed debug msg to avoid nullpointerexception when running mediawiki maintenance/importDump script
handle null values
📝 WalkthroughWalkthroughThe PR updates package ownership metadata, adds new hook handlers for parser initialization and multi-content saves, introduces subobject support in semantic data translation, standardizes property ID handling across the codebase, and adds EDTF time value and local media translation capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (16)
src/Translation/UserDefinedProperties.php (2)
47-49: Debug logging and formatting issues.
- Missing space before
else(should be} else {)- Debug message prefix "swb:" is inconsistent with other files using "SWB:"
♻️ Proposed fix
- }else{ - wfDebug("swb: cannot translate ".$propertyInfo['type']); + } else { + wfDebug("SWB: cannot translate ".$propertyInfo['type']); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/UserDefinedProperties.php` around lines 47 - 49, In UserDefinedProperties.php update the wfDebug call formatting and prefix: replace the "}else{" block with "} else {" and change the debug message prefix from "swb:" to "SWB:" in the wfDebug invocation that logs $propertyInfo['type'] so it matches the project's logging style and spacing conventions.
64-66: Misleading parameter name:$NumericPropertyIdis a string, not aNumericPropertyIdobject.The parameter
$NumericPropertyIdat line 64 is astringtype, not an instance of theNumericPropertyIdclass. This naming is confusing and violates the convention that PascalCase names are typically reserved for class names. Consider renaming to$propertyIdStringor simply$propertyIdto accurately reflect its type.♻️ Proposed fix
-private static function idFromWikibaseString( string $NumericPropertyId ): string { - return '___SWB_' . $NumericPropertyId; +private static function idFromWikibaseString( string $propertyId ): string { + return '___SWB_' . $propertyId; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/UserDefinedProperties.php` around lines 64 - 66, The parameter name $NumericPropertyId in the private static function idFromWikibaseString is misleading because it is a string, not a class; rename the parameter to a clear snake/camel case string name (e.g., $propertyId or $propertyIdString) in the function signature and update every usage inside idFromWikibaseString and any internal calls to idFromWikibaseString to use the new name; keep the string type hint intact and update any related docblocks/comments to match the new parameter name.src/EntryPoints/HookHandlers.php (2)
63-77: Minor issues inonMultiContentSave.
Unused variable (line 70):
$subjectis assigned but never used. Remove it or use it.Commented-out code (line 71): Remove the commented code if no longer needed.
Inconsistent debug prefix: "SWB:" is used but some messages have space issues.
♻️ Proposed cleanup
public static function onMultiContentSave( RenderedRevision $renderedRevision, UserIdentity $user, CommentStoreComment $summary, $flags, Status $hookStatus ) { wfDebug( __METHOD__ . "SWB: onMultiContentSave" ); $revision = $renderedRevision->getRevision(); - + $titleFactory = MediaWikiServices::getInstance()->getTitleFactory(); $title = $titleFactory->newFromLinkTarget($revision->getPageAsLinkTarget()); wfDebug( __METHOD__ . "SWB: onMultiContentSave...title:".$title ); - $subject = DIWikiPage::newFromTitle( $title ); - #$new_content = $revision->getContent(SlotRecord::MAIN, RevisionRecord::RAW)->getNativeData(); $semanticData = StoreFactory::getStore()->getSemanticData( DIWikiPage::newFromTitle( $title ) ); wfDebug( __METHOD__ . "SWB: onMultiContentSave: ".json_encode($semanticData) ); SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run( $semanticData ); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EntryPoints/HookHandlers.php` around lines 63 - 77, In onMultiContentSave, remove the unused $subject assignment and the commented-out line that fetches $new_content (they're dead code), and normalize the wfDebug messages to use a consistent "SWB:" prefix spacing (e.g., include a single space after method name before "SWB:") so logs are uniform; keep the existing $revision, $title, $semanticData usage and ensure SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run($semanticData) remains unchanged.
31-35: Typo in comment and unused parameter.Line 32 has a typo: "aktivates" should be "activates".
The
$parserparameter is unused (as flagged by static analysis). This is acceptable for hook handlers where the signature is defined by MediaWiki, but consider adding@suppressannotation or using/**@noinspectionPhpUnusedParameterInspection */if your codebase uses such conventions.♻️ Fix typo
public static function onParserFirstCallInit( Parser $parser ) { - # getInstance() will call initProperties() which aktivates existing hook onSmwInitProperties() + # getInstance() will call initProperties() which activates existing hook onSmwInitProperties() wfDebug( __METHOD__ . "SWB: onParserFirstCallInit" ); PropertyRegistry::getInstance(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EntryPoints/HookHandlers.php` around lines 31 - 35, Fix the typo in the comment inside onParserFirstCallInit by changing "aktivates" to "activates", and address the unused $parser parameter warning: either add a suppression annotation (e.g. a DocBlock `@suppress` or /** `@noinspection` PhpUnusedParameterInspection */) above the onParserFirstCallInit method or explicitly mark the parameter as unused in the body (e.g. (void)$parser) while leaving the call to PropertyRegistry::getInstance() intact.src/SemanticWikibase.php (2)
40-46: Inconsistent indentation detected.Lines 41 and 44-45 use spaces for indentation while the rest of the file uses tabs. This should be consistent.
♻️ Fix indentation
public function getSemanticDataUpdate(): SemanticDataUpdate { - $entityLookup = WikibaseRepo::getEntityLookup(); + $entityLookup = WikibaseRepo::getEntityLookup(); return new SemanticDataUpdate( $this->getTranslatorFactory(), - new LegacyAdapterItemLookup( $entityLookup ), - new LegacyAdapterPropertyLookup( $entityLookup ) + new LegacyAdapterItemLookup( $entityLookup ), + new LegacyAdapterPropertyLookup( $entityLookup ) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SemanticWikibase.php` around lines 40 - 46, The indentation in getSemanticDataUpdate is inconsistent: lines creating $entityLookup and the LegacyAdapter* constructor arguments use spaces while the rest of the file uses tabs; update the indentation so those lines use tabs to match the file's style (adjust the lines inside getSemanticDataUpdate around WikibaseRepo::getEntityLookup(), new LegacyAdapterItemLookup($entityLookup), and new LegacyAdapterPropertyLookup($entityLookup) to use tabs instead of spaces).
65-76: Debug logging should be cleaned up before production release.Multiple
wfDebugcalls have been added that log detailed information including JSON-encoded property arrays. While useful for development, this verbose logging should be removed or guarded behind a debug flag before production deployment:
- Line 65: Logs all properties on every registration call
- Lines 75-76: Logs every individual property ID and type
Consider removing these debug statements or wrapping them in a configuration check.
♻️ Suggested cleanup
public function registerProperties( PropertyRegistry $propertyRegistry ) { - wfDebug("SWB: register properties ".json_encode($this->getAllProperties())); foreach ( $this->getAllProperties() as $property ) { $propertyRegistry->registerProperty( $property->getId(), $property->getType(), $property->getLabel(), true, `#is_visible` true, `#is_annotable` false `#is_declarative` ); - wfDebug("SWB: register property ".$property->getId()); - wfDebug("SWB: register property ".$property->getType()); foreach ( $property->getAliases() as $alias ) { $propertyRegistry->registerPropertyAlias( $property->getId(), $alias ); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SemanticWikibase.php` around lines 65 - 76, Remove or guard the verbose wfDebug calls added around property registration: delete the wfDebug("SWB: register properties ".json_encode($this->getAllProperties())); and the per-property wfDebug("SWB: register property ".$property->getId()) / wfDebug("SWB: register property ".$property->getType()) lines, or wrap them in a runtime debug/config check so they only run when debugging is explicitly enabled; locate the loop that calls $propertyRegistry->registerProperty(...) iterating over $this->getAllProperties() and update the wfDebug invocations accordingly.src/SemanticDataUpdate.php (1)
66-73: Excessive debug logging with duplicate object construction.
Performance concern:
NumericPropertyIdis constructed twice - once for logging (line 68) and once for the actual lookup (line 72). This is inefficient.Debug cleanup needed: These verbose debug statements should be removed before production. JSON-encoding the entire
Titleobject and lookup results could impact performance and potentially leak sensitive information to logs.♻️ Proposed fix - remove debug logging or consolidate
private function getSemanticEntityForPropertyTitle( Title $title ): SemanticEntity { - wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($title)); - wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($title->getText())); - wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode(new NumericPropertyId( $title->getText() ))); - wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($this->propertyLookup->getPropertyForId( new NumericPropertyId( $title->getText() ) ))); - return $this->newPropertyTranslator( $title )->translateProperty( $this->propertyLookup->getPropertyForId( new NumericPropertyId( $title->getText() ) ) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SemanticDataUpdate.php` around lines 66 - 73, The code uses multiple wfDebug calls that JSON-encode the Title and construct NumericPropertyId twice, causing duplicate object construction and verbose/sensitive logging; remove or consolidate these wfDebug lines and create a single NumericPropertyId instance (e.g., $pid = new NumericPropertyId($title->getText())) to pass to propertyLookup->getPropertyForId and to any remaining debug output, then call $this->newPropertyTranslator($title)->translateProperty($pidProperty) using the retrieved property (from propertyLookup->getPropertyForId($pid)); avoid JSON-encoding entire Title or lookup results in production logs.src/SMW/SemanticEntity.php (1)
18-31: Misleading parameter names:$NumericPropertyIdis a string, not a class instance.Throughout this file, parameters named
$NumericPropertyIdare typed asstring, not as instances of theNumericPropertyIdclass. This naming convention is confusing because PascalCase typically denotes class names. Consider renaming to$propertyIdor$propertyIdStringfor clarity.♻️ Proposed renaming
-public function addPropertyValue( string $NumericPropertyId, SMWDataItem $dataItem ) { - $this->dataItemsPerProperty[$NumericPropertyId][] = $dataItem; +public function addPropertyValue( string $propertyId, SMWDataItem $dataItem ): void { + $this->dataItemsPerProperty[$propertyId][] = $dataItem; } -public function addSubobject( string $NumericPropertyId, Subobject $subobject ): void { - $this->subObjectsPerProperty[$NumericPropertyId][] = $subobject; +public function addSubobject( string $propertyId, Subobject $subobject ): void { + $this->subObjectsPerProperty[$propertyId][] = $subobject; } /** - * `@param` string $NumericPropertyId + * `@param` string $propertyId * `@return` SMWDataItem[] */ -public function getDataItemsForProperty( string $NumericPropertyId ): array { - return $this->dataItemsPerProperty[$NumericPropertyId] ?? []; +public function getDataItemsForProperty( string $propertyId ): array { + return $this->dataItemsPerProperty[$propertyId] ?? []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SMW/SemanticEntity.php` around lines 18 - 31, Parameters named $NumericPropertyId in methods addPropertyValue, addSubobject and getDataItemsForProperty are misleading because they are typed as string; rename them to a clearer name (e.g. $propertyId) across the method signatures, docblocks and all internal uses (array keys like $this->dataItemsPerProperty[$NumericPropertyId] and $this->subObjectsPerProperty[$NumericPropertyId]) to keep naming consistent; update any callers within the class/file to use the new parameter name and adjust PHPDoc annotations to reflect `@param` string $propertyId (or $propertyIdString) so PascalCase is not used for a simple string ID.src/Translation/StatementTranslator.php (3)
67-67: Add type hint for$statementNrparameter.For consistency with
strict_typesdeclaration and type safety.♻️ Proposed fix
- public function statementToSubobjects( Statement $statement, DIWikiPage $subject , $statementNr): array { + public function statementToSubobjects( Statement $statement, DIWikiPage $subject, int $statementNr ): array {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementTranslator.php` at line 67, Update the signature of the method statementToSubobjects to add a scalar type hint for $statementNr (e.g., int $statementNr) to match strict_types and improve type safety; modify the declaration in class StatementTranslator (method statementToSubobjects) and then update any call sites that pass non-int values to cast or ensure they pass an int so the code compiles and behavior remains unchanged.
88-93: Remove unused variables.
$prop,$propertyDV, and$propertyDIare defined but never used. This appears to be leftover code from development.♻️ Proposed fix
- #$prop = DIProperty::newFromUserLabel($mainSnak->getPropertyId()->getLocalPart()); - $prop = new DIProperty( 'semanticProperty:value'); - wfDebug('swb: prop: '.$prop); - $propertyDV = DataValueFactory::getInstance()->newPropertyValueByLabel("value"); - $propertyDI = $propertyDV->getDataItem(); - #$statementObj->addDataValue( DataValueFactory::getInstance()->newDataValueByItem($mainSnakDI, $propertyDI));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementTranslator.php` around lines 88 - 93, Remove the dead variables introduced in StatementTranslator.php: delete the unused declarations and debug call for $prop, and the unused $propertyDV and $propertyDI variables (the lines creating new DIProperty('semanticProperty:value'), wfDebug('swb: prop: '.$prop), DataValueFactory::getInstance()->newPropertyValueByLabel("value"), and ->getDataItem()). Ensure no other code depends on $prop, $propertyDV, or $propertyDI (e.g., references from $statementObj->addDataValue are commented out) and run tests to confirm no side effects.
129-141: Remove debug statements from production code.Multiple
wfDebugcalls (lines 130, 134, 138) should be removed. If logging is needed for troubleshooting, consider using structured logging at an appropriate level.Also, the static call
StatementTranslator::valueDIToString()on lines 77 and 109 should useself::valueDIToString()for a private static method.♻️ Proposed fix
private static function valueDIToString($di) : string { - wfDebug('smw: translate DI: '.get_class($di)); if( $di instanceof DIWikiPage ) { return $di->getTitle()->getText().""; } else if ( $di instanceof SMWDITime){ - wfDebug('time:'.$di->getSerialization()); return $di->asDateTime()->format( 'Y-m-d H:i:s' ); - `#return` "01.01.1700"; } - wfdebug('default'); return $di.""; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementTranslator.php` around lines 129 - 141, Remove all debug logging calls from the private static valueDIToString method (delete wfDebug and wfdebug usages) so production code has no stray debug prints; keep the conversion logic for DIWikiPage and SMWDITime intact (references: valueDIToString, DIWikiPage, SMWDITime). Also replace any external calls that invoke the private static method via StatementTranslator::valueDIToString with self::valueDIToString to use the proper private static access (references: StatementTranslator::valueDIToString → self::valueDIToString).src/Translation/StatementListTranslator.php (5)
40-40: Add type hint for$statementNrparameter.The parameter lacks a type declaration. Since the file uses
strict_types, addingintensures type safety and consistency.♻️ Proposed fix
- private function addStatement( SemanticEntity $semanticEntity, Statement $statement, $statementNr ): void { + private function addStatement( SemanticEntity $semanticEntity, Statement $statement, int $statementNr ): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` at line 40, Add an int type hint to the $statementNr parameter on the addStatement method to match strict_types and ensure type safety: update the method signature of addStatement(SemanticEntity $semanticEntity, Statement $statement, $statementNr): void to declare $statementNr as int, and adjust any callers if necessary to pass an integer (or cast/validate) so the signature change doesn't break usage.
12-16: Remove unused imports.
MWException,PropertyRegistry,StoreFactory, andStoreare imported but not used in this file.♻️ Proposed fix
-use MWException; -use SMW\PropertyRegistry; use SMW\Subobject; -use SMW\StoreFactory; -use SMW\Store;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 12 - 16, Remove the unused imports from StatementListTranslator.php: delete the use statements for MWException, SMW\PropertyRegistry, SMW\StoreFactory, and SMW\Store so only required imports (e.g., SMW\Subobject) remain; update the top-of-file use block in the StatementListTranslator class to drop those four unused symbols.
31-34: Inconsistent indentation.Lines 31 and 34 use spaces while the rest of the file uses tabs. Align indentation for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 31 - 34, The indentation in StatementListTranslator.php is inconsistent: the block in the method iterating over $statements uses spaces on the lines with "$i = 1;" and "$i++;" while the file uses tabs elsewhere; update the indentation in that loop to use tabs to match the file's style (e.g., align the lines around the foreach and the calls to $this->addStatement and $i++), keeping the logic in the method (the foreach over $statements->getBestStatements()->getByRank and the call to addStatement($semanticEntity, $statement, $i)) unchanged.
69-83: Remove or document commented-out code.This large block of commented-out qualifier handling code adds noise. If it's planned for future implementation, consider adding a TODO comment explaining the intent and removing the dead code. The qualifier handling is already implemented in
statementToSubobjects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 69 - 83, Remove the large commented-out qualifier handling block in StatementListTranslator (the block referencing $qualifiers, $statementTranslator->snakToDataItem, $semanticEntity->addPropertyValue and the ad-hoc newPropId logic) to reduce noise; if the logic is intended for future work, replace the commented code with a concise TODO comment referencing the intended behavior and pointing to the existing statementToSubobjects implementation that already handles qualifiers so reviewers know where qualifier handling lives.
87-89: Method name should use camelCase.
NumericPropertyIdForStatementuses PascalCase, but PHP convention for method names is camelCase. This also affects the call sites on lines 50 and 56.♻️ Proposed fix
- private function NumericPropertyIdForStatement( Statement $statement ): string { + private function numericPropertyIdForStatement( Statement $statement ): string { return UserDefinedProperties::idFromWikibaseProperty( $statement->getPropertyId() ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 87 - 89, Rename the method NumericPropertyIdForStatement to camelCase (e.g., numericPropertyIdForStatement) and update all call sites that invoke NumericPropertyIdForStatement to the new name; keep the method visibility and behavior identical (it should still call UserDefinedProperties::idFromWikibaseProperty($statement->getPropertyId()) and accept a Statement $statement), and update any related docblocks/tests or references to the old name so there are no lingering usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 2: The composer metadata is inconsistent: the "name"
(baillyk/semantic-wikibase) and "homepage" were changed to a personal fork but
the "support" URLs still point to ProfessionalWiki/SemanticWikibase; either
revert the "name" and "homepage" changes to match upstream or update the
"support" entries to the fork's URLs so package metadata is consistent. Locate
the "name" and "homepage" keys and the "support" object in composer.json and
make them all refer to the same package identity (either the original
ProfessionalWiki/SemanticWikibase values or the fork's values) before merging.
In `@src/EntryPoints/HookHandlers.php`:
- Around line 55-61: Fix onPageSaveComplete: import MediaWiki\Page\WikiPage (add
a use) and replace fully-qualified parameter types like
MediaWiki\User\UserIdentity with the imported UserIdentity; obtain semantic data
the same way as onMultiContentSave by calling
StoreFactory::getStore()->getSemanticData(DIWikiPage::newFromTitle($wikiPage->getTitle()));
then call the class method self::onSmwUpdateDataBefore($store, $semanticData)
(where $store is the Store returned by StoreFactory::getStore()) instead of the
bare onSmwUpdateDataBefore(null, ...). Ensure symbols referenced are
onPageSaveComplete, onSmwUpdateDataBefore, StoreFactory, Store, DIWikiPage, and
WikiPage.
In `@src/SMW/SemanticEntity.php`:
- Around line 22-24: Fix the inconsistent class name casing in the addSubobject
method: change the parameter type hint from SubObject to match the imported
class name Subobject so the signature public function addSubobject(string
$NumericPropertyId, Subobject $subobject) aligns with the import; update any
other occurrences in the method or nearby signatures that mistakenly use
SubObject to ensure consistent use of Subobject across the file.
- Around line 68-74: The add() method in SemanticEntity merges only
dataItemsPerProperty but ignores subObjectsPerProperty, causing subobjects to be
lost during functionalMerge(); update SemanticEntity::add(self $entity) to also
iterate over $entity->subObjectsPerProperty and merge each subobject into
$this->subObjectsPerProperty (preserving keys/indices and avoiding duplicates),
using the existing addSubobject/merge helper if present or by calling the same
internal merge logic used elsewhere for subobjects so subObjectsPerProperty is
properly combined alongside dataItemsPerProperty.
- Around line 48-54: The loop creates a DIProperty instance ($property = new
DIProperty($NumericPropertyId)) that is never used; either remove that line or
actually use $property to associate subobjects with the property before calling
$semanticData->addSubobject. Locate the loop over $this->subObjectsPerProperty
in SemanticEntity::(method containing this code), then either delete the unused
$property creation (DIProperty) or modify the inner loop to reference $property
when adding subobjects (e.g., associate or pass property information to
$semanticData->addSubobject) so $property is not unused.
In `@src/Translation/DataValueTranslator.php`:
- Around line 39-43: The debug statements in DataValueTranslator.php are logging
raw translated payloads (e.g. wfDebug calls that print
$typedValue->getValue()->getValue() and similar output around the StringValue
branch and lines ~121-130), which can leak sensitive data; remove or replace
those wfDebug calls so they do not include raw values—either drop the value
content entirely and only log safe context (like get_class($value) or
$propertyType) or log a non-reversible fingerprint (hash) instead; update the
StringValue branch and the other debug locations to use the sanitized approach
and keep the existing wfDebug context messages only.
- Around line 120-141: translateEDTF() declares a non-nullable \SMWDITime return
but returns null inside the Interval branch when neither hasStartDate() nor
hasEndDate(); replace that null with a valid \SMWDITime instance (created via
the \SMWDITime constructor or an available factory method such as a
"zero/unknown" or epoch sentinel) so translateEDTF always returns a \SMWDITime
even for empty Interval objects (update the code inside the Interval else branch
that currently does "return null").
- Line 68: Replace the invalid variable-variable expression `$$value.toString()`
with a proper method call on the TimeValue object: call the object's toString()
via the PHP object operator (e.g. $value->toString()) in the wfDebug call inside
DataValueTranslator (where TimeValue is logged) so the debug line uses a valid
method invocation on $value.
In `@src/Translation/StatementTranslator.php`:
- Around line 74-77: The code calls
StatementTranslator::valueDIToString($mainSnakDI) without guarding against
snakToDataItem returning null; add a null check after $mainSnakDI =
$this->snakToDataItem($mainSnak, $statement, $subject) and handle the null case
(skip creating the statement semantic subobject or use a safe fallback) before
calling valueDIToString; apply the same pattern for qualifier handling where
snakToDataItem is used (e.g., the variables created around the qualifier block
near lines 108–109) so you never pass a null $di into valueDIToString.
- Around line 46-56: The method snakToDataItem currently type-hints
PropertyValueSnak which prevents handling non-value qualifier Snak types; change
the parameter type to the base Snak (add use Wikibase\DataModel\Snak\Snak;) so
the method accepts any Snak, then keep the existing instanceof check
(PropertyValueSnak) inside snakToDataItem to guard calls to
snakWithSimpleDataValueToDataItem; leave the
containerValueTranslator->supportsStatement and statementToDataItem path
unchanged. Ensure the signature and import are updated and no other callers rely
on the narrower type.
---
Nitpick comments:
In `@src/EntryPoints/HookHandlers.php`:
- Around line 63-77: In onMultiContentSave, remove the unused $subject
assignment and the commented-out line that fetches $new_content (they're dead
code), and normalize the wfDebug messages to use a consistent "SWB:" prefix
spacing (e.g., include a single space after method name before "SWB:") so logs
are uniform; keep the existing $revision, $title, $semanticData usage and ensure
SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run($semanticData)
remains unchanged.
- Around line 31-35: Fix the typo in the comment inside onParserFirstCallInit by
changing "aktivates" to "activates", and address the unused $parser parameter
warning: either add a suppression annotation (e.g. a DocBlock `@suppress` or /**
`@noinspection` PhpUnusedParameterInspection */) above the onParserFirstCallInit
method or explicitly mark the parameter as unused in the body (e.g.
(void)$parser) while leaving the call to PropertyRegistry::getInstance() intact.
In `@src/SemanticDataUpdate.php`:
- Around line 66-73: The code uses multiple wfDebug calls that JSON-encode the
Title and construct NumericPropertyId twice, causing duplicate object
construction and verbose/sensitive logging; remove or consolidate these wfDebug
lines and create a single NumericPropertyId instance (e.g., $pid = new
NumericPropertyId($title->getText())) to pass to
propertyLookup->getPropertyForId and to any remaining debug output, then call
$this->newPropertyTranslator($title)->translateProperty($pidProperty) using the
retrieved property (from propertyLookup->getPropertyForId($pid)); avoid
JSON-encoding entire Title or lookup results in production logs.
In `@src/SemanticWikibase.php`:
- Around line 40-46: The indentation in getSemanticDataUpdate is inconsistent:
lines creating $entityLookup and the LegacyAdapter* constructor arguments use
spaces while the rest of the file uses tabs; update the indentation so those
lines use tabs to match the file's style (adjust the lines inside
getSemanticDataUpdate around WikibaseRepo::getEntityLookup(), new
LegacyAdapterItemLookup($entityLookup), and new
LegacyAdapterPropertyLookup($entityLookup) to use tabs instead of spaces).
- Around line 65-76: Remove or guard the verbose wfDebug calls added around
property registration: delete the wfDebug("SWB: register properties
".json_encode($this->getAllProperties())); and the per-property wfDebug("SWB:
register property ".$property->getId()) / wfDebug("SWB: register property
".$property->getType()) lines, or wrap them in a runtime debug/config check so
they only run when debugging is explicitly enabled; locate the loop that calls
$propertyRegistry->registerProperty(...) iterating over
$this->getAllProperties() and update the wfDebug invocations accordingly.
In `@src/SMW/SemanticEntity.php`:
- Around line 18-31: Parameters named $NumericPropertyId in methods
addPropertyValue, addSubobject and getDataItemsForProperty are misleading
because they are typed as string; rename them to a clearer name (e.g.
$propertyId) across the method signatures, docblocks and all internal uses
(array keys like $this->dataItemsPerProperty[$NumericPropertyId] and
$this->subObjectsPerProperty[$NumericPropertyId]) to keep naming consistent;
update any callers within the class/file to use the new parameter name and
adjust PHPDoc annotations to reflect `@param` string $propertyId (or
$propertyIdString) so PascalCase is not used for a simple string ID.
In `@src/Translation/StatementListTranslator.php`:
- Line 40: Add an int type hint to the $statementNr parameter on the
addStatement method to match strict_types and ensure type safety: update the
method signature of addStatement(SemanticEntity $semanticEntity, Statement
$statement, $statementNr): void to declare $statementNr as int, and adjust any
callers if necessary to pass an integer (or cast/validate) so the signature
change doesn't break usage.
- Around line 12-16: Remove the unused imports from StatementListTranslator.php:
delete the use statements for MWException, SMW\PropertyRegistry,
SMW\StoreFactory, and SMW\Store so only required imports (e.g., SMW\Subobject)
remain; update the top-of-file use block in the StatementListTranslator class to
drop those four unused symbols.
- Around line 31-34: The indentation in StatementListTranslator.php is
inconsistent: the block in the method iterating over $statements uses spaces on
the lines with "$i = 1;" and "$i++;" while the file uses tabs elsewhere; update
the indentation in that loop to use tabs to match the file's style (e.g., align
the lines around the foreach and the calls to $this->addStatement and $i++),
keeping the logic in the method (the foreach over
$statements->getBestStatements()->getByRank and the call to
addStatement($semanticEntity, $statement, $i)) unchanged.
- Around line 69-83: Remove the large commented-out qualifier handling block in
StatementListTranslator (the block referencing $qualifiers,
$statementTranslator->snakToDataItem, $semanticEntity->addPropertyValue and the
ad-hoc newPropId logic) to reduce noise; if the logic is intended for future
work, replace the commented code with a concise TODO comment referencing the
intended behavior and pointing to the existing statementToSubobjects
implementation that already handles qualifiers so reviewers know where qualifier
handling lives.
- Around line 87-89: Rename the method NumericPropertyIdForStatement to
camelCase (e.g., numericPropertyIdForStatement) and update all call sites that
invoke NumericPropertyIdForStatement to the new name; keep the method visibility
and behavior identical (it should still call
UserDefinedProperties::idFromWikibaseProperty($statement->getPropertyId()) and
accept a Statement $statement), and update any related docblocks/tests or
references to the old name so there are no lingering usages.
In `@src/Translation/StatementTranslator.php`:
- Line 67: Update the signature of the method statementToSubobjects to add a
scalar type hint for $statementNr (e.g., int $statementNr) to match strict_types
and improve type safety; modify the declaration in class StatementTranslator
(method statementToSubobjects) and then update any call sites that pass non-int
values to cast or ensure they pass an int so the code compiles and behavior
remains unchanged.
- Around line 88-93: Remove the dead variables introduced in
StatementTranslator.php: delete the unused declarations and debug call for
$prop, and the unused $propertyDV and $propertyDI variables (the lines creating
new DIProperty('semanticProperty:value'), wfDebug('swb: prop: '.$prop),
DataValueFactory::getInstance()->newPropertyValueByLabel("value"), and
->getDataItem()). Ensure no other code depends on $prop, $propertyDV, or
$propertyDI (e.g., references from $statementObj->addDataValue are commented
out) and run tests to confirm no side effects.
- Around line 129-141: Remove all debug logging calls from the private static
valueDIToString method (delete wfDebug and wfdebug usages) so production code
has no stray debug prints; keep the conversion logic for DIWikiPage and
SMWDITime intact (references: valueDIToString, DIWikiPage, SMWDITime). Also
replace any external calls that invoke the private static method via
StatementTranslator::valueDIToString with self::valueDIToString to use the
proper private static access (references: StatementTranslator::valueDIToString →
self::valueDIToString).
In `@src/Translation/UserDefinedProperties.php`:
- Around line 47-49: In UserDefinedProperties.php update the wfDebug call
formatting and prefix: replace the "}else{" block with "} else {" and change the
debug message prefix from "swb:" to "SWB:" in the wfDebug invocation that logs
$propertyInfo['type'] so it matches the project's logging style and spacing
conventions.
- Around line 64-66: The parameter name $NumericPropertyId in the private static
function idFromWikibaseString is misleading because it is a string, not a class;
rename the parameter to a clear snake/camel case string name (e.g., $propertyId
or $propertyIdString) in the function signature and update every usage inside
idFromWikibaseString and any internal calls to idFromWikibaseString to use the
new name; keep the string type hint intact and update any related
docblocks/comments to match the new parameter name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
composer.jsonextension.jsonsrc/EntryPoints/HookHandlers.phpsrc/SMW/SemanticEntity.phpsrc/SemanticDataUpdate.phpsrc/SemanticWikibase.phpsrc/Translation/DataValueTranslator.phpsrc/Translation/ItemTranslator.phpsrc/Translation/PropertyTranslator.phpsrc/Translation/PropertyTypeTranslator.phpsrc/Translation/StatementListTranslator.phpsrc/Translation/StatementTranslator.phpsrc/Translation/UserDefinedProperties.phptests/TestFactory.phptests/Unit/Translation/ContainerValueTranslatorTest.phptests/Unit/Translation/DataValueTranslatorTest.phptests/Unit/Translation/ItemTranslatorTest.phptests/Unit/Translation/PropertyTranslatorTest.phptests/Unit/Translation/StatementTranslatorTest.php
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "professional-wiki/semantic-wikibase", | |||
| "name": "baillyk/semantic-wikibase", | |||
There was a problem hiding this comment.
Package metadata changes may cause issues if merged upstream.
The package name and homepage have been changed to point to a personal fork, but the support URLs (lines 32-34) still reference the original ProfessionalWiki/SemanticWikibase repository. This creates an inconsistency.
If this PR is intended to be merged back to the upstream repository, consider reverting these changes to maintain the original package identity. If this is a permanent fork, update the support URLs as well for consistency.
Also applies to: 15-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` at line 2, The composer metadata is inconsistent: the "name"
(baillyk/semantic-wikibase) and "homepage" were changed to a personal fork but
the "support" URLs still point to ProfessionalWiki/SemanticWikibase; either
revert the "name" and "homepage" changes to match upstream or update the
"support" entries to the fork's URLs so package metadata is consistent. Locate
the "name" and "homepage" keys and the "support" object in composer.json and
make them all refer to the same package identity (either the original
ProfessionalWiki/SemanticWikibase values or the fork's values) before merging.
| public static function onPageSaveComplete( WikiPage $wikiPage, MediaWiki\User\UserIdentity $user, string $summary, int $flags, MediaWiki\Revision\RevisionRecord $revisionRecord, MediaWiki\Storage\EditResult $editResult ) { | ||
| wfDebug( __METHOD__ . "SWB: onPageSaveComplete" ); | ||
| // Access the semantic data of the page via the Semantic MediaWiki API | ||
| $semanticData = \SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage); | ||
|
|
||
| onSmwUpdateDataBefore(null, $semanticData); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the HookHandlers.php file and check its imports and structure
find . -name "HookHandlers.php" -type fRepository: ProfessionalWiki/SemanticWikibase
Length of output: 111
🏁 Script executed:
# Check imports and full content of HookHandlers.php
head -30 "src/EntryPoints/HookHandlers.php"Repository: ProfessionalWiki/SemanticWikibase
Length of output: 821
🏁 Script executed:
# Search for onSmwUpdateDataBefore method definition in the file
rg "onSmwUpdateDataBefore" "src/EntryPoints/HookHandlers.php" -A 5 -B 2Repository: ProfessionalWiki/SemanticWikibase
Length of output: 755
🏁 Script executed:
# Search for WikiPage import
rg "use.*WikiPage" "src/EntryPoints/HookHandlers.php"Repository: ProfessionalWiki/SemanticWikibase
Length of output: 96
🏁 Script executed:
# Check if ParserHooks class exists and search for getSemanticDataForPage
find . -name "*.php" -type f | xargs rg "ParserHooks" | head -20Repository: ProfessionalWiki/SemanticWikibase
Length of output: 204
🌐 Web query:
SMW Semantic MediaWiki ParserHooks getSemanticDataForPage API documentation
💡 Result:
getSemanticDataForPage is not documented as a public SMW “external API” endpoint; if you’re trying to retrieve a page’s semantic facts via HTTP, the documented SMW APIs to use are:
api.php?action=browsebysubject&subject=Page title(returns the facts for a subject, incl. special properties and subobjects) [2]api.php?action=ask/api.php?action=askargs(run SMW queries and get serialized results) [2]
If you meant an internal PHP API (e.g., from an extension / parser hook callback) to get semantic data for the current parse, SMW’s documented pattern is to create ParserData from the Title and ParserOutput, then call $parserData->getSemanticData() (and optionally push it back via pushSemanticDataToParserOutput()) [1].
SMW parser functions (“parser hooks”) themselves are documented here (e.g., #set, #subobject, #ask, etc.) [3].
Critical: onPageSaveComplete has multiple bugs that will cause runtime errors.
-
Incorrect function call: Line 60 calls
onSmwUpdateDataBefore(null, $semanticData)as a bare function. This method exists in the class (defined at line 38) but must be called asself::onSmwUpdateDataBefore(). Additionally, the method signature expectsStore $storeas the first parameter, notnull. -
Non-existent method:
\SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage)is not a documented public API in Semantic MediaWiki. The standard pattern is to useStoreFactory::getStore()->getSemanticData(DIWikiPage::newFromTitle($wikiPage->getTitle())), similar to line 72 inonMultiContentSave. -
Missing import:
WikiPageis not imported but is used as a type hint on line 55. -
Redundant type hints: Parameters use fully-qualified names like
MediaWiki\User\UserIdentitybutUserIdentityis already imported at line 8.
🐛 Proposed fix
+use WikiPage;
-public static function onPageSaveComplete( WikiPage $wikiPage, MediaWiki\User\UserIdentity $user, string $summary, int $flags, MediaWiki\Revision\RevisionRecord $revisionRecord, MediaWiki\Storage\EditResult $editResult ) {
+public static function onPageSaveComplete( WikiPage $wikiPage, UserIdentity $user, string $summary, int $flags, \MediaWiki\Revision\RevisionRecord $revisionRecord, \MediaWiki\Storage\EditResult $editResult ): void {
wfDebug( __METHOD__ . "SWB: onPageSaveComplete" );
- // Access the semantic data of the page via the Semantic MediaWiki API
- $semanticData = \SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage);
-
- onSmwUpdateDataBefore(null, $semanticData);
+ $semanticData = StoreFactory::getStore()->getSemanticData( DIWikiPage::newFromTitle( $wikiPage->getTitle() ) );
+ self::onSmwUpdateDataBefore( StoreFactory::getStore(), $semanticData );
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 55-55: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$summary'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$flags'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$revisionRecord'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$editResult'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EntryPoints/HookHandlers.php` around lines 55 - 61, Fix
onPageSaveComplete: import MediaWiki\Page\WikiPage (add a use) and replace
fully-qualified parameter types like MediaWiki\User\UserIdentity with the
imported UserIdentity; obtain semantic data the same way as onMultiContentSave
by calling
StoreFactory::getStore()->getSemanticData(DIWikiPage::newFromTitle($wikiPage->getTitle()));
then call the class method self::onSmwUpdateDataBefore($store, $semanticData)
(where $store is the Store returned by StoreFactory::getStore()) instead of the
bare onSmwUpdateDataBefore(null, ...). Ensure symbols referenced are
onPageSaveComplete, onSmwUpdateDataBefore, StoreFactory, Store, DIWikiPage, and
WikiPage.
| public function addSubobject( string $NumericPropertyId, SubObject $subobject ){ | ||
| $this->subObjectsPerProperty[$NumericPropertyId][] = $subobject; | ||
| } |
There was a problem hiding this comment.
Case mismatch in type hint: SubObject should be Subobject.
The import at line 11 is SMW\Subobject but the type hint here uses SubObject with capital 'O'. While PHP class names are case-insensitive, this inconsistency can cause confusion and should be corrected for code quality.
🐛 Proposed fix
-public function addSubobject( string $NumericPropertyId, SubObject $subobject ){
+public function addSubobject( string $NumericPropertyId, Subobject $subobject ): void {
$this->subObjectsPerProperty[$NumericPropertyId][] = $subobject;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function addSubobject( string $NumericPropertyId, SubObject $subobject ){ | |
| $this->subObjectsPerProperty[$NumericPropertyId][] = $subobject; | |
| } | |
| public function addSubobject( string $NumericPropertyId, Subobject $subobject ): void { | |
| $this->subObjectsPerProperty[$NumericPropertyId][] = $subobject; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SMW/SemanticEntity.php` around lines 22 - 24, Fix the inconsistent class
name casing in the addSubobject method: change the parameter type hint from
SubObject to match the imported class name Subobject so the signature public
function addSubobject(string $NumericPropertyId, Subobject $subobject) aligns
with the import; update any other occurrences in the method or nearby signatures
that mistakenly use SubObject to ensure consistent use of Subobject across the
file.
| foreach ( $this->subObjectsPerProperty as $NumericPropertyId => $subobjects ) { | ||
| $property = new DIProperty( $NumericPropertyId ); | ||
|
|
||
| foreach ( $subobjects as $subobject ) { | ||
| $semanticData->addSubobject($subobject); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused variable $property in subobject loop.
The $property variable on line 49 is created but never used. The subobjects are added directly to $semanticData without referencing this property. Either remove the unused variable or use it if it was intended to associate subobjects with the property.
🐛 Proposed fix - remove unused variable
foreach ( $this->subObjectsPerProperty as $NumericPropertyId => $subobjects ) {
- $property = new DIProperty( $NumericPropertyId );
-
foreach ( $subobjects as $subobject ) {
$semanticData->addSubobject($subobject);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ( $this->subObjectsPerProperty as $NumericPropertyId => $subobjects ) { | |
| $property = new DIProperty( $NumericPropertyId ); | |
| foreach ( $subobjects as $subobject ) { | |
| $semanticData->addSubobject($subobject); | |
| } | |
| } | |
| foreach ( $this->subObjectsPerProperty as $NumericPropertyId => $subobjects ) { | |
| foreach ( $subobjects as $subobject ) { | |
| $semanticData->addSubobject($subobject); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SMW/SemanticEntity.php` around lines 48 - 54, The loop creates a
DIProperty instance ($property = new DIProperty($NumericPropertyId)) that is
never used; either remove that line or actually use $property to associate
subobjects with the property before calling $semanticData->addSubobject. Locate
the loop over $this->subObjectsPerProperty in SemanticEntity::(method containing
this code), then either delete the unused $property creation (DIProperty) or
modify the inner loop to reference $property when adding subobjects (e.g.,
associate or pass property information to $semanticData->addSubobject) so
$property is not unused.
| public function add( self $entity ): void { | ||
| foreach ( $entity->dataItemsPerProperty as $propertyId => $dataItems ) { | ||
| foreach ( $entity->dataItemsPerProperty as $NumericPropertyId => $dataItems ) { | ||
| foreach ( $dataItems as $dataItem ) { | ||
| $this->addPropertyValue( $propertyId, $dataItem ); | ||
| $this->addPropertyValue( $NumericPropertyId, $dataItem ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Subobjects are not merged in the add() method.
The add() method merges dataItemsPerProperty from another entity but does not merge subObjectsPerProperty. This could lead to data loss when using functionalMerge() on entities that contain subobjects.
🐛 Proposed fix
public function add( self $entity ): void {
foreach ( $entity->dataItemsPerProperty as $NumericPropertyId => $dataItems ) {
foreach ( $dataItems as $dataItem ) {
$this->addPropertyValue( $NumericPropertyId, $dataItem );
}
}
+ foreach ( $entity->subObjectsPerProperty as $NumericPropertyId => $subobjects ) {
+ foreach ( $subobjects as $subobject ) {
+ $this->addSubobject( $NumericPropertyId, $subobject );
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SMW/SemanticEntity.php` around lines 68 - 74, The add() method in
SemanticEntity merges only dataItemsPerProperty but ignores
subObjectsPerProperty, causing subobjects to be lost during functionalMerge();
update SemanticEntity::add(self $entity) to also iterate over
$entity->subObjectsPerProperty and merge each subobject into
$this->subObjectsPerProperty (preserving keys/indices and avoiding duplicates),
using the existing addSubobject/merge helper if present or by calling the same
internal merge logic used elsewhere for subobjects so subObjectsPerProperty is
properly combined alongside dataItemsPerProperty.
| wfDebug( 'swb: translate: '.get_class( $value ).' ptype: '.$propertyType ); | ||
| } | ||
|
|
||
| if ( $value instanceof StringValue ) { | ||
| wfDebug( 'swb: translate: '. $typedValue->getValue()->getValue() ); |
There was a problem hiding this comment.
Avoid logging raw translated values.
Lines 39/43/129 log raw payload content and humanized values. This can leak sensitive imported data and create noisy debug logs.
Also applies to: 121-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/DataValueTranslator.php` around lines 39 - 43, The debug
statements in DataValueTranslator.php are logging raw translated payloads (e.g.
wfDebug calls that print $typedValue->getValue()->getValue() and similar output
around the StringValue branch and lines ~121-130), which can leak sensitive
data; remove or replace those wfDebug calls so they do not include raw
values—either drop the value content entirely and only log safe context (like
get_class($value) or $propertyType) or log a non-reversible fingerprint (hash)
instead; update the StringValue branch and the other debug locations to use the
sanitized approach and keep the existing wfDebug context messages only.
| } | ||
|
|
||
| if ( $value instanceof TimeValue ) { | ||
| wfDebug( 'swb: translate time: '. $$value.toString() ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Translation/DataValueTranslator.php | sed -n '60,80p'Repository: ProfessionalWiki/SemanticWikibase
Length of output: 925
🏁 Script executed:
# Check the full context around line 68
head -n 80 src/Translation/DataValueTranslator.php | tail -n 25Repository: ProfessionalWiki/SemanticWikibase
Length of output: 880
🏁 Script executed:
# Search for TimeValue class definition to understand available methods
rg -n 'class TimeValue|interface TimeValue' --type phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 59
🏁 Script executed:
# Verify the syntax - check if $$ pattern exists in the file
rg -n '\$\$' src/Translation/DataValueTranslator.phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 140
🏁 Script executed:
# Find TimeValue class definition
rg -n 'class TimeValue|interface TimeValue' -A 10 --type phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 59
🏁 Script executed:
# Check methods available on TimeValue
rg -n 'public function' -A 2 --type php | grep -A 2 -B 2 'getTime\|toString'Repository: ProfessionalWiki/SemanticWikibase
Length of output: 59
🏁 Script executed:
# Look for translateTimeValue method implementation
rg -n 'translateTimeValue' src/Translation/DataValueTranslator.php -A 15Repository: ProfessionalWiki/SemanticWikibase
Length of output: 1356
🏁 Script executed:
# Check the null return type issue mentioned in scratchpad (Line 140)
head -n 145 src/Translation/DataValueTranslator.php | tail -n 20Repository: ProfessionalWiki/SemanticWikibase
Length of output: 762
🏁 Script executed:
# Find the method containing line 140
sed -n '100,150p' src/Translation/DataValueTranslator.php | cat -nRepository: ProfessionalWiki/SemanticWikibase
Length of output: 2022
🏁 Script executed:
# Look for method signatures with return types in that range
rg -n 'private function|public function' src/Translation/DataValueTranslator.php | grep -E '(1[0-3][0-9]|14[0-5])'Repository: ProfessionalWiki/SemanticWikibase
Length of output: 296
🏁 Script executed:
# Get more context - find what method contains line 140
head -n 145 src/Translation/DataValueTranslator.php | tail -n 50Repository: ProfessionalWiki/SemanticWikibase
Length of output: 1708
🏁 Script executed:
# Get the complete translateEDTF method
sed -n '120,160p' src/Translation/DataValueTranslator.php | cat -nRepository: ProfessionalWiki/SemanticWikibase
Length of output: 1730
Fix invalid TimeValue debug expression on Line 68.
$$value.toString() uses invalid variable-variable syntax and will cause a fatal error at runtime when translating TimeValue. This should be replaced with a valid method call on the TimeValue object.
Proposed fix
- if ( $value instanceof TimeValue ) {
- wfDebug( 'swb: translate time: '. $$value.toString() );
- return $this->translateTimeValue( $value );
- }
+ if ( $value instanceof TimeValue ) {
+ wfDebug( 'swb: translate time: ' . $value->getTime() );
+ return $this->translateTimeValue( $value );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/DataValueTranslator.php` at line 68, Replace the invalid
variable-variable expression `$$value.toString()` with a proper method call on
the TimeValue object: call the object's toString() via the PHP object operator
(e.g. $value->toString()) in the wfDebug call inside DataValueTranslator (where
TimeValue is logged) so the debug line uses a valid method invocation on $value.
| private function translateEDTF( String $value ): \SMWDITime { | ||
| wfDebug( 'swb: translate edtf' ); | ||
| $tvb = new TimeValueBuilder( EdtfFactory::newParser() ); | ||
| $tvArr = $tvb->edtfToTimeValues( $value ); | ||
| $parser = \EDTF\EdtfFactory::newParser(); | ||
| $parsingResult = $parser->parse($value); | ||
| wfDebug($parsingResult->isValid()); // true | ||
| $edtfValue = $parsingResult->getEdtfValue(); // \EDTF\EdtfValue | ||
| $humanizer = \EDTF\EdtfFactory::newHumanizerForLanguage( 'en' ); | ||
| wfDebug($humanizer->humanize($edtfValue)); // string | ||
| wfDebug(get_class($edtfValue)); // string | ||
| $result = null; | ||
|
|
||
| if ( $edtfValue instanceof Interval ) { | ||
| if($edtfValue->hasStartDate()){ | ||
| $edtfValue = $edtfValue->getStartDate(); | ||
| } else if ($edtfValue->hasEndDate()){ | ||
| $edtfValue = $edtfValue->getEndDate(); | ||
| } else { | ||
| wfDebug('ERROR: unable to translate empty edtf interval to smw date'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the function signature and null returns as suggested
cd src/Translation && rg -n -C3 'private function translateEDTF\(|return null;' DataValueTranslator.phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Also get the complete function to see all return statements and context
rg -n -A30 'private function translateEDTF' src/Translation/DataValueTranslator.php | head -60Repository: ProfessionalWiki/SemanticWikibase
Length of output: 1291
🏁 Script executed:
#!/bin/bash
# Check if there are any other return statements in this function
rg -n 'return' src/Translation/DataValueTranslator.php | grep -A5 -B5 'translateEDTF'Repository: ProfessionalWiki/SemanticWikibase
Length of output: 430
🏁 Script executed:
#!/bin/bash
# Get the complete function from translateEDTF
rg -n -A100 'private function translateEDTF' src/Translation/DataValueTranslator.php | head -120Repository: ProfessionalWiki/SemanticWikibase
Length of output: 2560
Replace return null with a valid \SMWDITime object on line 140.
translateEDTF() declares a non-nullable return type \SMWDITime but returns null when an Interval has neither a start nor end date. This will throw a TypeError in strict mode.
Suggested fix
} else {
wfDebug('ERROR: unable to translate empty edtf interval to smw date');
- return null;
+ return new \SMWDITime( SMWDITime::CM_GREGORIAN, 1970 );
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 123-123: Avoid unused local variables such as '$tvArr'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/DataValueTranslator.php` around lines 120 - 141,
translateEDTF() declares a non-nullable \SMWDITime return but returns null
inside the Interval branch when neither hasStartDate() nor hasEndDate(); replace
that null with a valid \SMWDITime instance (created via the \SMWDITime
constructor or an available factory method such as a "zero/unknown" or epoch
sentinel) so translateEDTF always returns a \SMWDITime even for empty Interval
objects (update the code inside the Interval else branch that currently does
"return null").
| public function snakToDataItem( PropertyValueSnak $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem { | ||
| if ( !( $snak instanceof PropertyValueSnak ) ) { | ||
| return null; | ||
| } | ||
|
|
||
| if ( $this->containerValueTranslator->supportsStatement( $statement ) ) { | ||
| return $this->containerValueTranslator->statementToDataItem( $statement, $subject ); | ||
| } | ||
|
|
||
| return $this->snakWithSimpleDataValueToDataItem( $mainSnak ); | ||
| return $this->snakWithSimpleDataValueToDataItem( $snak ); | ||
| } |
There was a problem hiding this comment.
Type mismatch: snakToDataItem type hint is too restrictive for qualifier handling.
The method signature declares PropertyValueSnak $snak, but qualifiers from $statement->getQualifiers() can include other Snak types (e.g., PropertyNoValueSnak, PropertySomeValueSnak). This causes a type error when non-PropertyValueSnak qualifiers are passed on lines 74 and 108.
The instanceof check on line 47 becomes redundant with the type hint but was likely intended to handle mixed Snak types.
🐛 Proposed fix: Accept base Snak type
- public function snakToDataItem( PropertyValueSnak $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem {
- if ( !( $snak instanceof PropertyValueSnak ) ) {
+ public function snakToDataItem( \Wikibase\DataModel\Snak\Snak $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem {
+ if ( !( $snak instanceof PropertyValueSnak ) ) {
return null;
}You'll also need to add the import:
use Wikibase\DataModel\Snak\Snak;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/StatementTranslator.php` around lines 46 - 56, The method
snakToDataItem currently type-hints PropertyValueSnak which prevents handling
non-value qualifier Snak types; change the parameter type to the base Snak (add
use Wikibase\DataModel\Snak\Snak;) so the method accepts any Snak, then keep the
existing instanceof check (PropertyValueSnak) inside snakToDataItem to guard
calls to snakWithSimpleDataValueToDataItem; leave the
containerValueTranslator->supportsStatement and statementToDataItem path
unchanged. Ensure the signature and import are updated and no other callers rely
on the narrower type.
| $mainSnakDI = $this->snakToDataItem($mainSnak, $statement, $subject); | ||
| // create a smw semantic subobject for the statement | ||
| $statementProperty = $mainSnak->getPropertyId()->getLocalPart(); | ||
| $statementValue = StatementTranslator::valueDIToString($mainSnakDI); |
There was a problem hiding this comment.
Potential null pointer dereference.
snakToDataItem can return null (e.g., for non-PropertyValueSnak qualifiers), but the result is passed directly to valueDIToString without null checking. This will cause errors when get_class($di) or string coercion is attempted on null.
🐛 Proposed fix: Add null check
$mainSnakDI = $this->snakToDataItem($mainSnak, $statement, $subject);
+ if ( $mainSnakDI === null ) {
+ return $result;
+ }
// create a smw semantic subobject for the statement
$statementProperty = $mainSnak->getPropertyId()->getLocalPart();
$statementValue = StatementTranslator::valueDIToString($mainSnakDI);And for qualifiers (around line 108):
$qualifierDI = $this->snakToDataItem($actQualifier, $statement, $subject);
+ if ( $qualifierDI === null ) {
+ continue;
+ }
$qualifierValue = StatementTranslator::valueDIToString($qualifierDI);Also applies to: 108-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/StatementTranslator.php` around lines 74 - 77, The code calls
StatementTranslator::valueDIToString($mainSnakDI) without guarding against
snakToDataItem returning null; add a null check after $mainSnakDI =
$this->snakToDataItem($mainSnak, $statement, $subject) and handle the null case
(skip creating the statement semantic subobject or use a safe fallback) before
calling valueDIToString; apply the same pattern for qualifier handling where
snakToDataItem is used (e.g., the variables created around the qualifier block
near lines 108–109) so you never pass a null $di into valueDIToString.
compatible to mediawiki version 1.39
added support of wb qualifiers
added support of datatype wikibaseLocalMedia
added basic support of datatype EDTF
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores